Skip to content

fix(runtime): free the admission slot when the deferred prelude panics - #138

Merged
ThomasK33 merged 3 commits into
mainfrom
fix/deferred-prelude-panic-release
Oct 29, 2026
Merged

ThomasK33 merged 3 commits into
mainfrom
fix/deferred-prelude-panic-release

Conversation

@ThomasK33

Copy link
Copy Markdown
Member

Why

Every open follow-up issue in coder/chat gets one small, reviewed PR, and this one closes #135. Under DispatchDeferred, a State panic in the prelude unwinds to net/http, which recovers it. dispatchDeferred freed the admission slot only when the prelude returned, so the panicked delivery kept its slot and its queue turn record, and Shutdown timed out.

What changed

  • dispatchDeferred frees the slot in a defer on every exit that hands no work to a tail, a panic included. The panic still reaches net/http.
  • The slot release is the sync.OnceFunc from fix(runtime): harden the queue turn record against a double release and a State panic #134, so no exit can release twice.
  • A new test serves the runtime behind a real httptest.Server. It panics State.AcquireLock under the queue strategy and State.MarkEvent under the burst strategy.

Scope

  • The fix covers every strategy, because the leak was in dispatchDeferred. Before the fix, a MarkEvent panic leaked the slot under drop, debounce, burst, and concurrent too.
  • A panic in a detached tail goroutine still ends the process, as on main.
  • The panicked event keeps its in-memory acceptance entry, so a redelivery of the same event ID waits until its own request ends. That leak has a different cause and is left as a follow-up.

Tradeoffs

  • A recover and re-panic would also free the slot. The defer with an ownership flag frees it and leaves the panic value and stack untouched.

Blast Radius

Only deferred dispatch changes. A delivery that resolves in the prelude now frees its slot as dispatchDeferred returns, not just before, which the caller cannot tell apart.

Verification

  • Before the fix, the new test fails in both cases with Shutdown ... context deadline exceeded. The queue case also keeps 1 turn record.
  • Three wrong fixes fail. A silent recover fails the new test with status 200. A release without OnceFunc trips the leaveQueueScope assert in the queue tests. A release without the ownership flag fails two admission tests.
  • The new test and the queue tests pass 20 of 20 runs under -race. go vet and go test pass in every workspace module, and go test -race passes for the root package.

Closes #135


Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: xhigh

…eferred prelude

Under DispatchDeferred, a State panic in the prelude runs on the net/http
goroutine. The test serves the runtime behind a real httptest.Server, panics
State.AcquireLock under the queue strategy and State.MarkEvent under the
burst strategy, and requires that no queue turn record stays behind and that
Shutdown drains before a 10s deadline. It fails on main: the panicked delivery
keeps its admission slot, so Shutdown returns context deadline exceeded.
dispatchDeferred freed the slot only when the prelude returned. A State panic
unwound past it to net/http, which recovers the panic, so the delivery kept
its slot and its queue turn record, and Shutdown timed out.

A deferred call now runs the release on every exit that hands no work to a
tail, a panic included. The panic still propagates to net/http. The release
is the sync.OnceFunc from #134, so no exit can release twice.
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-29T22:22:21.319616Z dfe6d9f Manual request
🔒 Security Review ✅ Completed 2026-10-29T22:22:33.276388Z dfe6d9f Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: dfe6d9f4a0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: dfe6d9f4a0

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33
ThomasK33 merged commit dc5726d into main Oct 29, 2026
2 checks passed
ThomasK33 added a commit that referenced this pull request Oct 30, 2026
…panics (#142)

## Why

Every open follow-up issue in coder/chat gets one small, reviewed PR, and this one closes #139. When `State` or the app's `slog` handler panics during the prelude, `net/http` recovers the panic, but three things the delivery took stay taken. A redelivery of the event waits on the open acceptance entry until its own request ends. The next delivery for the thread waits for `ThreadLockTTL`. A burst member whose join log panicked never gets a runner, so later members never run and `Shutdown` times out.

## What changed

- The prelude defers `abandonEventAcceptance` and its own `releaseOnResolve`. The first does nothing once the prelude finished the entry. The second does nothing once the lease is released or handed to the tail. So every release still runs once, and nothing recovers the panic.
- `joinBurstBatch` starts the burst runner in a `defer`. The runner still starts after the join log, and it now also starts when the log panics.
- `takeQueueTurn` deletes the superseded waiter before it logs. Without that, the released lease lets the old waiter take the lock behind the newer turn, and the `commitPending` assert ends the process under `DispatchDeferred`. ADR 0015 gains the rule as invariant 4.

## Scope

- Covers `State` and logger panics in the prelude and in the burst join, under both dispatch modes.
- A logger panic in the supersede log still loses events, because `MarkEvent` already ran and the platform's retry is deduped. Under debounce the newest event is lost. In `takeQueueTurn` both the newer event and the waiter it replaced are lost. That is event loss, not a leak, and is tracked in #143.
- The dispatch span of a panicked delivery stays open, as on `main`.

## Tradeoffs

- A `recover` and re-panic would also free each resource. The deferred releases leave the panic value and stack untouched, as #138 chose.
- Moving `go runBurstScope` above the join log would also fix the burst leak, but the runner could then log before the member's join line.

## Blast Radius

Only the prelude, the burst join, and the queue supersede path change. On every normal path the new defers find nothing left to release.

## Verification

- Each fix lands after its failing test. Before the fixes, the redelivery and the next delivery for the thread time out after 2 s, and the burst member never runs.
- Wrong fixes fail the new tests. A silent `recover` returns 200 or 500 instead of aborting the request. A second release reports `lock_release_failed` or panics with `close of closed channel`. A release after the handoff fails the existing lock tests, or admits a second burst member while the first holds the only slot. The lease release without the `takeQueueTurn` change trips the `commitPending` assert.
- The new tests and the Queue, Deferred, Burst, Admission, and Concurrent tests pass 20 of 20 runs under `-race`. `go vet` and `go test` pass in every workspace module, and `go test -race` passes for the root package.

Closes #139

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

runtime: a State panic in the deferred prelude leaks the queue turn record and admission slot

1 participant